Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@tturocy @rahulsavani This is now ready for review - there's a lot happening in this PR, so please take a look at the docs links I've added in the top comment of this PR which should explain how everything now works.

I'm going to create some additional feature issues for the catalog and add them to the list in #394

After your review, we can make a bunch of PRs to update the catalog entries for these purposes:

1. To update the names and any metadata for all the games in catalog.yml (and either remove or comment out any that aren't wanted) (i.e. following the "Add game files to the catalog" guide from the Updating the games catalog page)
2. To move games used in tests into the catalog_games.py (using the "Code games for the catalog" guide) and update the tests to load those games from the catalog

We should decide whether we want to merge this branch into master now and make those PRs there, or alternatively you could branch off enhancement/731 and make PRs that target this branch.

For the catalog interface, I had been thinking we could look more at R's datasets package https://stat.ethz.ch/R-manual/R-devel/library/datasets/html/00Index.html.

To parallel the load() function there, we'd have for example

pygambit.catalog.load_game("stripped_down_poker")

When the game is one that's just serialised to a file, that just becomes a simple load of that file. There's not really anything being gained by crafting classes manually - especially because we're going to have game names that don't map to valid Python identifiers (there's good reasons they might start with a digit).

This style of game naming (stripped_down_poker or stripped-down-poker) is similar to what a lot of papers use for referring to games or families of games (usually rendering the game name in fixed-with type). By tapping into that we would hope people would adopt our game naming as being "standard".

For parameterised games or games which are generated procedurally, this load call would need to hook into a register of generating functions. (It's my first thought that actually classes aren't the right way to build games and that what we currently have as classes really ought to be functions - and in any event composition is the right way to be thinking about how to build some of these game families and not inheritance. So while we might have classes in places in the generation of games, we shouldn't impose that as an architecture.).

Parameterised games could be expressed like

pygambit.catalog.load_game("nim", n=5)

So we'd have just a **kwargs on load_game, but then the individual game-generating functions would presumably specify which arguments are valid (and that's where argument checking would happen and appropriate exceptions generated on arguments that aren't meaningful).

As an additional note here just for memory - it will be useful for us to be able to generate reproducible sets of "random payoff" games. These will be useful for benchmarks, and we hope others would adopt them when they want to do algorithm horse-races. So we might have for example

pygambit.catalog.load_game("random_nfg", shape=[10, 10, 10], seed=67)

We do not need to do that right now, but the architecture above allows us to do this fairly easily (because with the seed our "random" game generation does become deterministic).

@tturocy thanks for your comment, I'm in agreement with your suggestion to use functions for each game instead of classes and I'm sure I can refactor it that way, especially if it's preferable to retain the game names that don't map to valid Python identifiers (such as those starting with numbers).

Part of the motivation of having the informatively named classes was so that the pygambit.catalog.games() function returned a list of classes where it was clear to the user what each game catalog entry was. Perhaps it could instead return a dictionary of the catalog "slug" (filename or function name for those defined in code) as keys and game titles as values:

{
  "stripped_down_poker": "Stripped-Down Poker: a simple game of one-card poker from Reiley et al (2008).",
  ...
}

... or better still might be a pandas dataframe so a user can easily scroll through a table. What do you think?

Another reason for the subclassing system is that the autogenerated set of classes look good on the API reference docs, the catalog's online documentation therefore automatically built. We can still have this if catalog entries are functions instead of classes: but in that case pygambit.catalog.load_game("stripped_down_poker") would need call pygambit.catalog.stripped_down_poker() to under the hood and pygambit.catalog.load_game("nim", n=5) call pygambit.catalog.nim(n=5). I guess that can work nicely, I think that's what you're getting at here:

So we'd have just a **kwargs on load_game, but then the individual game-generating functions would presumably specify which arguments are valid (and that's where argument checking would happen and appropriate exceptions generated on arguments that aren't meaningful).

Doing it that way means the only remaining use for catalog.yml would be to define metadata for games that fall outside what's included in a Game object, which can be used for searching the catalog like pygambit.catalog.games(metadata_field_xyz=7) - is this something that we definitely want?

If so, another idea I think you suggested was having an optional attribute called metadata on Game that accepts a dataclass. Difficulty there might be that any catalog entry function generated from a game file would need to be manually defined in code. But I suppose I could change catalog_update.py to populate catalog_games.py with a long list of similar looking functions (instead of creating the custom classes via the catalog.yaml as currently), which simply call read_efg("filename") or read_nfg("filename") and return the game object. We can then update those game functions manually to include metadata as required.

Closing this PR after discussions last week, see new ideas here: #731 (comment)